Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Align proto package paths and python generated package paths. #13

Merged
merged 7 commits into from
Jan 30, 2025
Merged

Align proto package paths and python generated package paths. #13

merged 7 commits into from
Jan 30, 2025

Conversation

Ricefrog
Copy link
Collaborator

@Ricefrog Ricefrog commented Jan 30, 2025 •
edited
Loading

Summary of changes

In python generated class context, all generated code *_pb2 is imported from keysight_chakra.infra or keysight_chakra.mlcommons

from keysight_chakra.infra import (
    infra_pb2,
    annotate_pb2,
)
from keysight_chakra.mlcommons import et_def_pb2

When compiling protos from this package with your own protos, .proto files import definitions like this:

import "keysight_chakra/infra/infra.proto";
import "keysight_chakra/infra/annotate.proto";
import "keysight_chakra/mlcommons/et_def.proto";

The imports are used in messages like this:

message Wrapper {
  keysight_chakra.infra.Infrastructure infrastructure = 1;
  repeated keysight_chakra.infra.Annotation infra_annotations = 2;
  keysight_chakra.mlcommons.Node node = 3;
}

ajbalogh
ajbalogh reviewed Jan 30, 2025
View reviewed changes
Copy link
Collaborator

@ajbalogh ajbalogh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm except for the makefile step to replace the package name

Makefile Outdated Show resolved Hide resolved
@ajbalogh
Copy link
Collaborator

I like the standardization on keysight_chakra.protobuf as the package name as it follows the google protobuf naming convention.

thomas-am
thomas-am reviewed Jan 30, 2025
View reviewed changes
Copy link
Collaborator

@thomas-am thomas-am left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a good example to follow:
https://github.com/googleapis/googleapis/tree/master/google

Except that we don't have a mono repo for all our protos in one place.

Here's the google chat API for example:
https://github.com/googleapis/googleapis/blob/master/google/chat/v1/chat_service.proto

See how it imports other .protos.
With that it would be something like keysight/chakra/v1alpha/*.proto

Versioning is something we should have, for example.
"protobuf" is only used for protobuf library related files.

@Ricefrog
Copy link
Collaborator Author

Ricefrog commented Jan 30, 2025 •
edited
Loading

This would be a good example to follow: https://github.com/googleapis/googleapis/tree/master/google

Except that we don't have a mono repo for all our protos in one place.

Here's the google chat API for example: https://github.com/googleapis/googleapis/blob/master/google/chat/v1/chat_service.proto

See how it imports other .protos. With that it would be something like keysight/chakra/v1alpha/*.proto

Versioning is something we should have, for example. "protobuf" is only used for protobuf library related files.

I see. Maybe we could do

keysight_chakra/infra/*.proto
keysight_chakra/mlcommons/et_def.proto

to make a distinction between our custom protos and et_def.proto, which we pull directly from mlcommons.

As for having versioning in the directory structure, is it something we could put off until we know there is active third-party consumption of these protos?

ajbalogh
ajbalogh reviewed Jan 30, 2025
View reviewed changes
Copy link
Collaborator

@ajbalogh ajbalogh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we including generated code? it should only be proto files - the generated code comes as part of the package at build time.

@Ricefrog
Copy link
Collaborator Author

why are we including generated code? it should only be proto files - the generated code comes as part of the package at build time.

I forgot to update .gitignore and accidentally pushed the generated code. I'll undo that

@Ricefrog Ricefrog merged commit 3b09e9c into main Jan 30, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomas-am thomas-am thomas-am left review comments

@ajbalogh ajbalogh ajbalogh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Ricefrog @ajbalogh @thomas-am